-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dont display tooltips on platforms that dont support hover #12168
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
# Conflicts: # package-lock.json # package.json # src/components/DatePicker/index.js
// Skip the tooltip and return the children, if the text is empty. | ||
if (_.isEmpty(this.props.text)) { | ||
// Skip the tooltip and return the children if the text is empty or the device does not support hovering | ||
if (_.isEmpty(this.props.text) || !this.hasHoverSupport) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most critical behavior change in this PR
@Santhosh-Sellavel @alex-mechler One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Ready for review. Note that there is a bug in the videos that also exists on main and in production: https://expensify.slack.com/archives/C049HHMV9SM/p1672706945551159 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better now, thank you!
Looks fine but will get to this again, tomorrow. |
// so that the top of the pointer aligns with the bottom of the component. | ||
// | ||
// Always add the manual vertical shift passed in as a parameter. | ||
top: shouldShowBelow ? (manualShiftVertical - POINTER_HEIGHT) : (tooltipHeight + manualShiftVertical), | ||
// OR if the pointer should be above the tooltip wrapper, then the pointer up (-) by the pointer's height | ||
// so that the bottom of the pointer lines up with the top of the tooltip | ||
top: shouldShowBelow ? -POINTER_HEIGHT : tooltipHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we removed manualShiftVertical
can you explain why it's not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-mechler just waiting for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cc: @tgolen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! What can I help with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @Santhosh-Sellavel. I think manualShiftVertical
wasn't really working so I removed it for testing, but I'm going to re-investigate this and see if it's needed or not and if I can fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer Checklist
Screenshots/VideosWeb & DesktopScreen.Recording.2023-01-08.at.1.18.29.AM.movMobile Web - Chrome & Mobile Web - SafariScreen.Recording.2023-01-08.at.1.42.50.AM.moviOS & AndroidScreen.Recording.2023-01-08.at.2.10.20.AM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, and works well. @Santhosh-Sellavel are you able to finish off the checklist?
bump @Santhosh-Sellavel ^ |
# Conflicts: # src/components/Tooltip/index.js
Conflicts resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tests well!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @roryabraham in version: 1.2.51-0 🚀
|
This has caused #14149, which I confirmed by reverting this pr locally. I've tried fixing this locally, but have been unable. We properly call to update the tooltip width, but are improperly calculating the width of it. |
🚀 Deployed to production by @Julesssss in version: 1.2.51-1 🚀
|
To follow up on the above, this was caused by the switch to |
Details
ReactDOM.createPortal
, which in turn:Fixed Issues
$ #9737
PROPOSAL: #9737 (comment)
Tests
Touch-screen devices
Web/Desktop (narrow screens)
Back
should appear.Back
should appear.Web/Desktop (wide screen)
n/a
QA Steps
Same as tests.
PR Review Checklist
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
Web
WebNarrowTooltip.mov
WebWideTooltip.mov
Mobile Web - Chrome
screen-20230102-183325.mp4
Mobile Web - Safari
RPReplay_Final1672708413.MP4
Desktop
DesktopNarrow.mov
DesktopWide.mov
iOS
RPReplay_Final1672709013.MP4
Android
video.mp4